Manual Combine Unloader — Call Unloader button for player-driven combines#1246
Manual Combine Unloader — Call Unloader button for player-driven combines#1246antler22 wants to merge 8 commits intoCourseplay:mainfrom
Conversation
Grain cart manual call button, US units, CP unload threshold dropped to 20%, pathfinding for grain cart
AIParameterSettingList.lua unit conversion stays in the personal fork/main but is not part of the manual combine unloader feature for upstream. Co-Authored-By: Claude Sonnet 4-6 <noreply@anthropic.com>
…f, remove unnecessary guards - Rename cpIsCallGrainCartActive -> cpIsManualCombineCallingUnloader per pvaiko's naming suggestion - Rename cpToggleCallGrainCart -> cpToggleManualUnloader for consistency - Rename CallGrainCartEvent -> CpManualUnloaderEvent (class + file) - Update translation key CP_callGrainCart -> CP_callManualUnloader; visible text now reads 'Call Unloader' - Rebase PurePursuitController.lua on current upstream base so PR diff shows only our actual additions (~35 lines) instead of the full file - Remove unnecessary if CollisionAvoidanceController / if ProximityController guards in AIDriveStrategyUnloadCombine.setAIVehicle() — these classes are always loaded - Revert MotorController.lua to upstream (changes were unrelated to this feature and crept in by mistake) Co-Authored-By: Claude Sonnet 4-6 <noreply@anthropic.com>
|
@pvaiko — reopening as a clean feature-only PR replacing #1243. All of your review feedback has been addressed:
Happy to discuss anything further or make adjustments. |
…C, use ImplementUtil.isChopper - Revert PathfinderConstraints.lua to upstream (getFruitCheckDimensions removed; change was too restrictive and geometrically unsound) - Revert translation_cz/ru/uk.xml to upstream (unintended edits) - Restore nil guard for spec.courseDisplay in CpCourseManager.lua (accidentally dropped) - Replace manual numAutoAimingStates chopper check with ImplementUtil.isChopper() in CpAIWorker.lua and CpFieldworkHudPage.lua - Add explanatory comment to CpAIFieldWorker:onUpdate() for the CP-active auto-deactivation logic - Simplify PPC off-track grace period: use g_time directly, remove onOffTrackShutdown() callback (pvaiko: AI slop) - Remove ppc.offTrackGracePeriodMs override and onOffTrackShutdown() from AIDriveStrategyUnloadCombine.lua; rely on existing disableStopWhenOffTrack() calls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, clean up guards - CpManualCombineProxy: replace callUnloader() stub with isActiveCpCombine() returning true (correct approach per reviewer) - AIDriveStrategyCombineCourse: revert to upstream — combine strategy not installed when manually driving - PurePursuitController: revert to upstream — disableStopWhenOffTrack(5000) in the strategy is sufficient, grace period is redundant - PathfinderUtil: revert to upstream — swath detection unconfirmed, defer to future iteration - AIDriveStrategyUnloadCombine: remove do/end wrapper and self.ppc guard from disableStopWhenOffTrack call, fold into isManualProxy check; revert fill-level block to upstream; remove nil guards from harvesterStrategy calls in handleChopperTurn; remove redundant or-6 fallback on getWorkWidth(); handle manual blocker inline at blocking vehicle check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
||
| -- combine stopped in the meanwhile, like for example end of course | ||
| if combineStrategy:willWaitForUnloadToFinish() then | ||
| self:debug('change to unload stopped combine') |
There was a problem hiding this comment.
can we please revert this to keep logging consistent (also, state is logged already)
| self:isInFrontAndAlignedToMovingCombine(true) | ||
| self:info('not in a good position to unload, cancelling rendezvous, trying to recover') | ||
| -- for some reason (like combine turned) we are not in a good position anymore then set us up again | ||
| self:info('unloadMovingCombine: EXIT - not in a good position (behind=%s, inFront=%s), startWaitingForSomethingToDo', |
There was a problem hiding this comment.
can we please revert this to keep logging consistent (also, state is logged already)
|
Thank you for addressing the comments so far, will continue the review as work permits... |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thank you, I appreciate your patience with this! |
np, still busy at work, but we'll get there.... |
|
Hello again, I've noticed that the CP unloader is getting released more frequently now, after I rolled back some changes. What do you make of the advice from AI on this, is there a way to accomplish this without putting the guards back in that you advised against? 
|
Love this :) But it is right in 1., that's a good catch. I think the easiest would be to call disableStopWhenOffTrack() from the proxy's update() function (unconditionally). I'd try with that change first and if we still have the problem, take a look at the log. 2 and 3 seems bogus to me. |
| local spec = CpAIFieldWorker.getSpec(self) | ||
| if not spec then return end | ||
| if spec.cpManualCombineProxy then | ||
| CpUtil.debugVehicle(CpDebug.DBG_FIELDWORK, self, 'Call grain cart deactivated') |
There was a problem hiding this comment.
Can we change the debug text here and below from "call grain cart" to manual combine or whatever we called it, for consistency?
| ---@return AIDriveStrategyCombineCourse|CpManualCombineProxy|nil | ||
| function AIDriveStrategyUnloadCombine:getCombineStrategy() | ||
| if self.combineToUnload then | ||
| -- Try the CP drive strategy directly without requiring getIsCpActive() to be true. |
There was a problem hiding this comment.
Don't think this comment makes any sense.
| if strategy then return strategy end | ||
| end | ||
| -- Fall back to the manual combine proxy | ||
| if self.combineToUnload.cpGetManualCombineProxy then |
There was a problem hiding this comment.
I think it would be safer flip this and do this first, then the strategy, as I'm not 100% when the strategy is removed after the combine driving with CP is stopped.
There was a problem hiding this comment.
makes sense, I'll have it flipped.
| --- calculated from the offset harvester course). | ||
| --- This goal point is calculated from the harvester's position. It is on a straight line parallel to the harvester, | ||
| --- under the pipe and look ahead distance ahead of the unloader | ||
| --- driveBesideCombine() creates this goal when approaching the harvester to align with the pipe better and faster than |
| if dz > 5 then | ||
| if dz > 5 or isManual then | ||
| _, _, dz = localToLocal(self.vehicle:getAIDirectionNode(), self:getPipeOffsetReferenceNode(), 0, 0, 0) | ||
| -- For manual combines: use the vehicle's natural (non-CTE-adjusted) lookahead distance. |
There was a problem hiding this comment.
The preferred solution here is to add a getNormalLookaheadDistance() to PPC and use it for both the CP and manual cases (so just change self.ppc:getLookaheadDistance() to self.ppc:getNormalLookaheadDistance().
And add something like "always use getters to access member variables" to AGENTS.md :)
| self.followingCourseOffset = self:getFollowingCourseOffset(self.combineToUnload) | ||
| self.followCourse:setOffset(self.followingCourseOffset, 0) | ||
| self.reverseForTurnCourse = nil | ||
| -- Initialise the refresh timer so the first periodic refresh fires 5 s from now, |
There was a problem hiding this comment.
This whole logic should be in setupFollowCourse().
There was a problem hiding this comment.
Moving logic into setupFollowCourse()
| -- current position and pointing in the combine's current heading. | ||
| local combineStrategy = self:getCombineStrategy() | ||
| if combineStrategy and combineStrategy.isManualProxy and combineStrategy:isManualProxy() then | ||
| local combineX, _, combineZ = getWorldTranslation(self.combineToUnload:getAIDirectionNode()) |
There was a problem hiding this comment.
Also, since this is never driven, there is no need for all these, just use Course.createStraightForwardCourse(self.vehicle, 100)
There was a problem hiding this comment.
simplifying to createStraightForwardCourse.
| self.reverseForTurnCourse = nil | ||
| -- Initialise the refresh timer so the first periodic refresh fires 5 s from now, | ||
| -- not on the very first update frame. | ||
| self.followCourseRefreshTime = g_time |
There was a problem hiding this comment.
This does not seem to be used anywhere (don't think there is a need to refresh anyway)
| --- doesn't cause excessive penalty and the search terminates much faster | ||
| --- - caps maxIterations at the default 40 000 instead of the potentially huge | ||
| --- field-polygon-scaled value, avoiding multi-second searches on large maps | ||
| function AIDriveStrategyUnloadCombine:startPathfindingToWaitingCombine(xOffset, zOffset, failureCallback, isManualCombine) |
There was a problem hiding this comment.
I have quite a few issues with the changes in this function, but in general, I'd prefer not making any change to the pathfinder. I understand the intent, but if we do something like that, it should apply globally, we may introduce a global or per vehicle setting for the pathfinder, something like "strict"/"lenient", but would not hack one single state for a specific use case.
There was a problem hiding this comment.
Ok. With the other changes going in, I'll revert this for now and see how pathfinder performs.
| self.combineToUnload:getCpDriveStrategy():reconfirmRendezvous() | ||
| self:getCombineStrategy():reconfirmRendezvous() | ||
|
|
||
| -- Every ~10 s, redirect toward the combine's live position without stopping, but ONLY if the |
There was a problem hiding this comment.
We can't do this. This fundamentally changes the behavior of the unloader by overriding the pathfinder and will just drive through everything that happens to be between the combine and the unloader.
I understand the intent but this is the key problem: when the combine is moving, the unloader should drive to a future position of the combine, not the current one.
CP solve this by the combine estimating where it will be full and asking the unloader to drive there. This is not a trivial thing and often fails, but the combine's behavior is predictable as it drives on the CP course.
With the manually driven combine we now have no estimates. So if the combine is moving and especially if it is far away, driving to its current position will simply be wrong, and I understand we are trying to adjust for that here.
I don't have a good solution for this. The manual combine could calculate some rough estimates, which may work within a row but since we don't know the pattern it is driving, anything after a turn would be inaccurate.
There was a problem hiding this comment.
Understand your comment here. IRL, driver doesn't always predict as well as CP solver to find "where" the best unload spot would be, they tend to follow and guess. I wanted to balance "ok, combine moved, let's find again quickly" with "lets not recalculate every second and overload someones CPU".
In my opinion, I'm happy enough with the "go to point at time of call" logic for this use case. If the location difference is extreme or drive is long, a user could/should toggle driver off, then on again as he closes in. I don't think that's a big ask for a user, and only needed for large fields. For turns, a user SHOULD be toggling "off" until a turn is completed. Perhaps that just takes coaching/experience, not ideal, but it's a start. I'll keep thinking of a better way to do this.
There was a problem hiding this comment.
Trying this fix...
"The cleanest replacement: when the cart is in DRIVING_TO_COMBINE and the combine has moved more than ~25m from where we last pathfound, re-trigger the proper A* pathfinder. This uses all the existing terrain/crop constraints and doesn't require any new logic — just a periodic re-dispatch. I'll implement that instead."
There was a problem hiding this comment.
Yeah, that sounds like a good compromise, however, do this only with the manual combine, not with the CP.
| @@ -61,25 +61,28 @@ function CollisionAvoidanceController:findPotentialCollisions() | |||
| if AIDriveStrategyCombineCourse.isActiveCpCombine(vehicle) then | |||
There was a problem hiding this comment.
and not vehicle:cpIsManualCombineCallingUnloader() here would be a lot cleaner and more readable than the check for strategy.
- Fix debug strings: 'Call grain cart' -> 'manual unloader' (CpAIFieldWorker) - getCombineStrategy(): proxy-first order, remove misleading comment - driveBesideCombine(): update doc comment; use ppc:getNormalLookaheadDistance() getter - PurePursuitController: add getNormalLookaheadDistance() getter - Add AGENTS.md: 'always use getters to access member variables' - setupFollowCourse(): move manual-combine placeholder logic here, use Course.createStraightForwardCourse - startCourseFollowingCombine(): remove now-inlined placeholder block and unused followCourseRefreshTime init - Revert startPathfindingToWaitingCombine to near-upstream (no isManualCombine param/tuning) - Revert call site and onPathfindingDoneToWaitingCombine redirect-timer init - Revert driveToCombine() straight-line redirect block - CollisionAvoidanceController: exclude manual combines from course-intersection check; revert to direct getCpDriveStrategy() call - getDriveData(): disable PPC off-track check unconditionally when manual proxy is active; remove redundant call from unloadMovingCombine Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the combine moves ≥30 m from the position recorded at pathfinding start, trigger a fast A* re-path to its new location. Guards prevent unnecessary stops: - checks only every 5 s - skips if <20 m of course remains (almost there) - skips if already within 25 m of combine (proximity handling takes over) - caps maxIterations at HybridAStar.defaultMaxIterations for speed - excludes the combine from obstacles so the path aims behind it Uses onPathfindingFailedToMovingTarget so a failure is a soft retry, not a job-stopper. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
This feature adds a "Call Unloader" button to manually-driven combines (i.e. combines that the player is driving themselves, not Courseplay-controlled). When activated, a nearby Courseplay-managed grain cart automatically:
The design goal is that the player only touches the button once — the grain cart handles everything until the pipe is closed.
Files Changed
scripts/ai/CpManualCombineProxy.luaAIDriveStrategyCombineCourseinterfacescripts/specializations/CpAIFieldWorker.luacpToggleManualUnloader/cpIsManualCombineCallingUnloadertoggle and proxy lifecyclescripts/specializations/CpAIWorker.luascripts/ai/strategies/AIDriveStrategyUnloadCombine.luascripts/ai/PurePursuitController.luascripts/pathfinder/PathfinderUtil.luahasFruit()config/VehicleSettingsSetup.xmlscripts/events/CpManualUnloaderEvent.luaKey Design Decisions
CpManualCombineProxyimplements the fullAIDriveStrategyCombineCourseinterface soAIDriveStrategyUnloadCombinecan interact with a manual combine using identical method calls — no nil checks or special-casing scattered through the unloader code.isManualProxy() → true— marker method used to gate manual-only behavior inAIDriveStrategyUnloadCombinewithout re-querying the vehicle.getFillLevelPercentage() → 1— always reports full so fill level never causes the cart to leave. The only valid exit isisUnloadFinished()(pipe closed for 2+ seconds).driveBesideCombine()direct goal point — for manual combines, a live goal point is computed every frame from the combine's AI direction node at the pipe's lateral offset,normalLookAheadDistancemeters ahead. As the combine turns, the node rotates and the cart follows naturally through curves and gentle S-bends. The placeholder course (built from combine's current heading) exists only to satisfy PPC initialisation and is never consulted for steering.PPC soft-recovery hook —
onOffTrackShutdown()inAIDriveStrategyUnloadCombinetransitions the cart to IDLE instead of callingstopCurrentAIJob. The proxy'scallUnloaderWhenNeeded()re-summons within ~2.5 s. The user never has to manually restart the grain cart.Off-track grace period —
offTrackGracePeriodMs = 10000(class default, unchanged). During active unloading,disableStopWhenOffTrack(5000)is called each tick, fully suppressing off-track detection while the cart is under the pipe.Forage harvesters excluded — button and keybind are hidden when
pipeSpec.numAutoAimingStates > 0. Their auto-aim spout geometry requires a separate implementation.Backward Compatibility
All changes are backward-compatible with CP-driven combines:
combineStrategy:isManualProxy()callUnloaderPercentslider change (min 60% → 20%) applies to all combinesKnown Limitations